-
Notifications
You must be signed in to change notification settings - Fork 23
DRAFT: Add global default authorizers for core-internal workflows #1215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
DRAFT: Add global default authorizers for core-internal workflows #1215
Conversation
| _authorizers = Authorizers() | ||
|
|
||
|
|
||
| def get_authorizers() -> Authorizers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this to reduce footgun opportunities since this is security-focused. On the other hand, it doesn't match how we handle app_settings, so I'm open to not going this route if folks have strong opinions about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the this need to be registered from the app the same way we have app.register_authorization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that this doesn't belong to the settings class.
What I do think may cause confusion for end users is the AuthManager class which is defined in the oauth2_lib library:
class AuthManager:
"""Manages the authentication and authorization mechanisms for the application.
This manager class orchestrates authentication and authorization, utilizing OpenID Connect (OIDC) and
Open Policy Agent (OPA) respectively. It serves as a central hub for user authentication states and
authorization policies. If defaults are insufficient, users can register alternatives or customize existing ones.
...."""An instance of that class is created in the OrchestratorCore(FastAPI) class, and in downstream apps the authn/authz instances can be overridden like so:
app.register_authentication(surf_authn)
app.register_authorization(surf_authz)
app.register_graphql_authorization(surf_graphql_authz)The AuthManager's docstring suggests that it manages all authn/authz, but obviously this only holds for the API - the workflow authz is specifically for the Worker (which, in case of the threadpool executor, is running in the same process; but for the celery executor it will be separate).
I'm not suggesting we should make the Authorizers part of the AuthManager, maybe we can just name/document things well enough to avoid confusion. And perhaps tweak the AuthManager's docstring a little bit to say it's for the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auth manager isn't specifically for the API, I would have used it in our version for the authorize_callback if we could use an async function. I'm also not saying that they need to be included in the AuthManager within oauth2-lib, since its orchestrator specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the workflow authz is specifically for the Worker
Just to clarify - the API endpoints for creating and resuming processes do check these callbacks, along with a couple other REST and GraphQL endpoints. I'm not actually aware of the worker running these checks; by checking in the request handler, we can prevent the workflow from ever making it to the queue if the process shouldn't be queued/re-queued.
I originally considered adding OrchestratorCore.register_internal_authorization_callbacks(authorize_callback=None, retry_auth_callback=None). I decided against it at the time since the process of checking this info seems kinda independent of the app from the perspective of building orch-core. Something-something loose coupling. That said, I can see it being more intuitive for the end user to configure it as part of the application.
Should I add it to OrchestratorCore class, or just continue with the current approach while making it clear that AuthManager is a separate thing in the docs?
orchestrator/settings.py
Outdated
| expose_settings("oauth2lib_settings", oauth2lib_settings) # type: ignore | ||
|
|
||
|
|
||
| class Authorizers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could have gone into AppSettings, but it seemed like
- it would be weird printing these out when settings are exposed
- (more importantly) it would suggest to users that this can be set by environment variable
So I opted to separate them out. Feedback/direction appreciated.
b185180 to
1291dd9
Compare
CodSpeed Performance ReportMerging #1215 will not alter performanceComparing Summary
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1215 +/- ##
==========================================
- Coverage 79.58% 79.34% -0.25%
==========================================
Files 272 273 +1
Lines 13196 13365 +169
Branches 1295 1311 +16
==========================================
+ Hits 10502 10604 +102
- Misses 2414 2480 +66
- Partials 280 281 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This adds a configurable collection of Authorizer callbacks for downstream users to assign to. Presently, this enables end users to define authZ callbacks for workflows defined upstream in orchestrator-core. This could also be extended to include default Authorizers for user-defined workflows.
Marking as draft because this change should also update the docs, but I want some initial feedback on the approach before I do that. I've already created some comments explaining my approach, but I'm very open to adjusting per feedback.
Closes #1066